Skip to content

[FEAT] Add function to get app version (CU #86b3a5vyh)#5

Merged
fermuch merged 10 commits intomainfrom
feat/add-command-to-get-app-version#86b3a5vyh
Jan 31, 2025
Merged

[FEAT] Add function to get app version (CU #86b3a5vyh)#5
fermuch merged 10 commits intomainfrom
feat/add-command-to-get-app-version#86b3a5vyh

Conversation

@Vinocis
Copy link
Collaborator

@Vinocis Vinocis commented Jan 8, 2025 •
edited by github-actions bot
Loading

PR Type

enhancement, tests

Description

  • Added version/0 function to retrieve app versions.

  • Enhanced error handling with detailed error messages.

  • Updated internal processing functions for better operation handling.

  • Added tests for the new version/0 function.

Changes walkthrough 📝

Relevant files
Enhancement
numscriptex.ex
Add version function and improve error handling                   

lib/numscriptex.ex

  • Added version/0 function to return app versions.
  • Improved error handling with detailed messages.
  • Refactored internal processing functions.
    		• +44/-8   
    Tests
    numscriptex_test.exs
    Add tests for version function and error messages               

    test/numscriptex_test.exs

  • Added tests for version/0 function.
  • Updated tests to check detailed error messages.
    		• +17/-3   
    Additional files
    numscript.wasm [link]   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @Vinocis Vinocis requested a review from fermuch January 8, 2025 17:14
    @Vinocis Vinocis self-assigned this Jan 8, 2025
    @github-actions
    Copy link

    github-actions bot commented Jan 8, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The new version/0 function and the process function enhancements should be reviewed to ensure that error handling is robust and consistent with the rest of the codebase.

    def version do
  numscriptex_version =
    :numscriptex
    |> Application.spec(:vsn)
    |> to_string()

  {:ok, numscript_wasm_version} = process(:version)

  {:ok, Map.put(numscript_wasm_version, :numscriptex, numscriptex_version)}
end
    JSON Decoding

    The maybe_decode_json function has been introduced to handle JSON decoding based on operation type. Ensure that this logic correctly handles all expected cases and does not introduce any regressions.

    defp maybe_decode_json(result, :version), do: result
defp maybe_decode_json(result, _operation), do: JSON.decode(result)

    @github-actions
    Copy link

    github-actions bot commented Jan 8, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Handle potential errors from the process(:version) call to prevent crashes

    Ensure that the process/2 function handles the case where process(:version) returns
    an error, to prevent the version/0 function from crashing.

    lib/numscriptex.ex [72-74]

    -{:ok, numscript_wasm_version} = process(:version)
+case process(:version) do
+  {:ok, numscript_wasm_version} -> {:ok, Map.put(numscript_wasm_version, :numscriptex, numscriptex_version)}
+  {:error, reason} -> {:error, reason}
+end
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a critical issue where the version/0 function could crash if process(:version) returns an error. By handling the error case, the code becomes more robust and reliable, preventing potential runtime failures.

    		9

    fermuch
    fermuch requested changes Jan 21, 2025
    View reviewed changes
    @Vinocis Vinocis requested a review from fermuch January 21, 2025 17:34
    fermuch
    fermuch requested changes Jan 21, 2025
    View reviewed changes
    Copy link
    Contributor

    @fermuch fermuch left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Como é necessário ter o arquivo WASM para poder executar os testes, é necessário completar #4 antes de poder continuar este PR.

    @Vinocis Vinocis requested a review from fermuch January 30, 2025 18:08
    fermuch
    fermuch requested changes Jan 31, 2025
    View reviewed changes
    Copy link
    Contributor

    @fermuch fermuch left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Ao tentar rodar acontece o seguinte:

    image

    == Compilation error in file lib/numscriptex.ex ==
** (File.Error) could not make directory (with -p) "/home/fermuch/Documentos/Dev/AYVU/numscriptex/_build/dev/lib/numscriptex/priv": file already exists
    (elixir 1.18.0) lib/file.ex:346: File.mkdir_p!/1
    lib/numscriptex.ex:52: (module)

    fermuch
    fermuch requested changes Jan 31, 2025
    View reviewed changes
    @Vinocis Vinocis requested a review from fermuch January 31, 2025 15:21
    fermuch
    fermuch approved these changes Jan 31, 2025
    View reviewed changes
    Copy link
    Contributor

    @fermuch fermuch left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM!

    @fermuch fermuch merged commit c7636bd into main Jan 31, 2025
    4 checks passed
    @fermuch fermuch deleted the feat/add-command-to-get-app-version#86b3a5vyh branch January 31, 2025 18:39
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @fermuch fermuch fermuch approved these changes

    Assignees

    @Vinocis Vinocis

    Labels

    Review effort [1-5]: 3

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @Vinocis @fermuch